Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unused variable error when OpenMP is disabled #6529

Merged
merged 2 commits into from
Dec 10, 2023

Conversation

sitic
Copy link
Contributor

@sitic sitic commented Dec 9, 2023

Type

Motivation and Context

error: unused variable 'kInnerThreads' [-Werror,-Wunused-variable]

type of errors occur when building without OpenMP on macOS Sonoma.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

Adds (void)kOuterThreads; style fixes to suppress the warnings.

Occurs on macOS Sonoma when building without OpenMP.

Fixes isl-org#6514
Copy link

update-docs bot commented Dec 9, 2023

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

@saurabheights
Copy link
Contributor

saurabheights commented Dec 9, 2023

My C++ is very rusty, so just use these as suggestions (I could be wrong). Ideally, if a variable is not used, it should not be even declared. Furthermore, casting to void is too cryptic. There might be a better alternative where openmp related code is defined in #ifdef block - https://stackoverflow.com/questions/24488239/use-variable-from-cmake-in-c

Also, check for other OpenMP code in the library. There might be similar approach used.

@sitic
Copy link
Contributor Author

sitic commented Dec 10, 2023

Thank you for your suggestions, @saurabheights.

(void)variable; is a common pattern used to suppress the unused variable warnings, which are escalated to errors here. While it’s not the most elegant solution (in C++17 we could use [[maybe_unused]]), it's the most suitable approach in this case, in my opinion. I've added a code comment to clarify their purpose.

Indeed ideally unused variables should not be declared. Typically the number of threads will be computed in the OpenMP specific #pragma omp parallel line, however, in these two cases, they are separated out into new lines to manage line length. I don’t believe enclosing them in an #ifdef block is appropriate here, as it would make the code harder to read and more complex. They don’t do any meaningful work if OpenMP is disabled and will be optimized away, so it boils down to a preference in style.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sitic we are planning to switch from C++14 to C++17 soon (some MSVC compatibility issues still remain), but this is a good workaround in the meantime.

@ssheorey ssheorey merged commit df34629 into isl-org:main Dec 10, 2023
36 checks passed
@sitic sitic deleted the unused-variables branch December 11, 2023 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build error during build from source on Sonoma M1 Max
3 participants